fix(preingestion): create unified BFB with 0o600 perms#3688
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Summary by CodeRabbit
WalkthroughThe BFB copier now builds unified pre-ingestion artifacts through a path-parameterized writer, enforces ChangesUnified BFB assembly
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
2 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/preingestion-manager/src/bfb_rshim_copier.rs`:
- Around line 185-203: Remove the destination-existence guard around the unified
BFB assembly so every invocation regenerates credentials for the current
machine. Update the write flow near the unified artifact creation and the
corresponding logic around the later referenced section to write complete
content to a fresh temporary file with mode 0o600, then atomically rename it
over the destination. Extend the relevant test to pre-populate stale credentials
and permissions, invoke the copier, and verify the destination is fully replaced
with current content and secure permissions.
- Around line 174-178: Update the artifact creation flow described near the
bf.cfg security comment to call set_permissions(0o600) on the opened file before
writing the payload, rather than relying on open’s .mode(0o600) alone. Add a
regression case using a restrictive umask and verify the resulting artifact
remains owner-readable and writable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c76aa5cd-db78-41d8-aceb-adb12f3be995
📒 Files selected for processing (1)
crates/preingestion-manager/src/bfb_rshim_copier.rs
Comment on lines
+185
to
+203
| if fs::metadata(unified_bfb_path).await.is_err() { | ||
| tracing::info!(path = unified_bfb_path, "Writing unified preingestion BFB"); | ||
| let bf_cfg_contents = format!( | ||
| "BMC_USER=\"{username}\"\nBMC_PASSWORD=\"{password}\"\nBMC_REBOOT=\"yes\"\nCEC_REBOOT=\"yes\"\n" | ||
| ); | ||
|
|
||
| let mut preingestion_bfb = File::open(PREINGESTION_BFB_PATH).await.map_err(|err| { | ||
| BfbRshimCopyError::Other { | ||
| details: format!("failed to open {PREINGESTION_BFB_PATH}: {err}"), | ||
| } | ||
| })?; | ||
|
|
||
| let mut unified_bfb = | ||
| File::create(UNIFIED_PREINGESTION_BFB_PATH) | ||
| let mut preingestion_bfb = | ||
| File::open(source_bfb_path) | ||
| .await | ||
| .map_err(|err| BfbRshimCopyError::Other { | ||
| details: format!("failed to create {UNIFIED_PREINGESTION_BFB_PATH}: {err}"), | ||
| details: format!("failed to open {source_bfb_path}: {err}"), | ||
| })?; | ||
|
|
||
| let mut unified_bfb = OpenOptions::new() | ||
| .write(true) | ||
| .create(true) | ||
| .truncate(true) | ||
| .mode(0o600) | ||
| .open(unified_bfb_path) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Always replace the destination; the existence check reuses stale credentials.
After the first call, Line 185 skips assembly entirely. Because callers provide machine-specific BMC credentials, subsequent machines receive the first artifact’s password; partial or insecure existing files are also accepted.
Atomically replace the artifact using a fresh 0o600 temporary file and rename it into place. Extend the test by pre-populating the destination with stale credentials and permissions, then verify complete replacement.
Also applies to: 290-330
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/preingestion-manager/src/bfb_rshim_copier.rs` around lines 185 - 203,
Remove the destination-existence guard around the unified BFB assembly so every
invocation regenerates credentials for the current machine. Update the write
flow near the unified artifact creation and the corresponding logic around the
later referenced section to write complete content to a fresh temporary file
with mode 0o600, then atomically rename it over the destination. Extend the
relevant test to pre-populate stale credentials and permissions, invoke the
copier, and verify the destination is fully replaced with current content and
secure permissions.
wminckler
reviewed
Jul 17, 2026
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3688.docs.buildwithfern.com/infra-controller |
wminckler
approved these changes
Jul 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The unified pre-ingestion BFB embeds the BMC root password in cleartext via its bf.cfg blob, but was created with File::create (0o666 & ~umask, typically 0644), leaving the secret world-readable to any local user/process.
Create the artifact with OpenOptions::mode(0o600) so permissions are set atomically at creation and independent of umask. Extract the assembly logic into a testable helper and add a unit test asserting the artifact is owner-only.
Related issues
#3060
Type of Change
Breaking Changes
Testing
Additional Notes